Migrate withReportAndPrivateNotesOrNotFound from withOnyx to useOnyx #49238
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Sep 21, 2024
Reviewer Checklist
Screenshots/VideosAndroid: Native49238-android-native.mp4Android: mWeb Chrome49238-android-chrome.mp4iOS: Native49238-ios-native.mp4iOS: mWeb Safari49238-ios-safari.mp4MacOS: Chrome / Safari49238-web.mp4MacOS: Desktop49238-desktop.mp4 |
blazejkustra
suggested changes
Sep 25, 2024
| export default function (pageTitle: TranslationPaths) { | ||
| // eslint-disable-next-line rulesdir/no-negated-variables | ||
| return <TProps extends WithReportAndPrivateNotesOrNotFoundProps, TRef>( | ||
| WrappedComponent: ComponentType<TProps & RefAttributes<TRef>>, | ||
| ): React.ComponentType<Omit<Omit<TProps, keyof WithReportAndPrivateNotesOrNotFoundOnyxProps> & RefAttributes<TRef>, keyof WithReportOrNotFoundOnyxProps>> => { | ||
| ): React.ComponentType<Readonly<Omit<TProps & RefAttributes<TRef>, keyof WithReportOrNotFoundOnyxProps>>> => { |
There was a problem hiding this comment.
Wrapped component should also omit WithReportAndPrivateNotesOrNotFoundOnyxProps while in here you are including it. I think there is a mistake in withReportOrNotFound (report property is added to both onyx and normal props). Because of that you will get type errors when correctly typeing the component.
I prepared a diff for you, could you try and use it?
edit: this resulted in many ts errors, working on a different approach 😮💨
There was a problem hiding this comment.
thanks for your suggestion, i'll try to use this soon
There was a problem hiding this comment.
I think I got it this time!
diff --git a/src/pages/home/report/withReportAndPrivateNotesOrNotFound.tsx b/src/pages/home/report/withReportAndPrivateNotesOrNotFound.tsx
index 420526fa38b..e6875acd799 100644
--- a/src/pages/home/report/withReportAndPrivateNotesOrNotFound.tsx
+++ b/src/pages/home/report/withReportAndPrivateNotesOrNotFound.tsx
@@ -17,17 +17,20 @@ import {isEmptyObject} from '@src/types/utils/EmptyObject';
import type {WithReportOrNotFoundOnyxProps, WithReportOrNotFoundProps} from './withReportOrNotFound';
import withReportOrNotFound from './withReportOrNotFound';
-type WithReportAndPrivateNotesOrNotFoundProps = WithReportOrNotFoundProps & {
+type WithReportAndPrivateNotesOrNotFoundOnyxProps = {
+ /** Session of currently logged in user */
session: OnyxEntry<Session>;
};
+type WithReportAndPrivateNotesOrNotFoundProps = WithReportOrNotFoundProps & WithReportAndPrivateNotesOrNotFoundOnyxProps;
+
export default function (pageTitle: TranslationPaths) {
// eslint-disable-next-line rulesdir/no-negated-variables
return <TProps extends WithReportAndPrivateNotesOrNotFoundProps, TRef>(
WrappedComponent: ComponentType<TProps & RefAttributes<TRef>>,
- ): React.ComponentType<Readonly<Omit<TProps & RefAttributes<TRef>, keyof WithReportOrNotFoundOnyxProps>>> => {
+ ): React.ComponentType<Omit<Omit<TProps, keyof WithReportAndPrivateNotesOrNotFoundOnyxProps> & React.RefAttributes<TRef>, keyof WithReportOrNotFoundOnyxProps>> => {
// eslint-disable-next-line rulesdir/no-negated-variables
- function WithReportAndPrivateNotesOrNotFound(props: TProps, ref: ForwardedRef<TRef>) {
+ function WithReportAndPrivateNotesOrNotFound(props: Omit<TProps, keyof WithReportAndPrivateNotesOrNotFoundOnyxProps>, ref: ForwardedRef<TRef>) {
const {translate} = useLocalize();
const {isOffline} = useNetwork();
const [session] = useOnyx(ONYXKEYS.SESSION);
@@ -80,7 +83,7 @@ export default function (pageTitle: TranslationPaths) {
return (
<WrappedComponent
// eslint-disable-next-line react/jsx-props-no-spreading
- {...props}
+ {...(props as TProps)}
ref={ref}
session={session}
/>
|
@akinwale i updated and test well, please check again |
roryabraham
previously requested changes
Sep 27, 2024
| const {translate} = useLocalize(); | ||
| const {isOffline} = useNetwork(); | ||
| const {route, report, session} = props; | ||
| const [session] = useOnyx(ONYXKEYS.SESSION); |
There was a problem hiding this comment.
It looks like only accountID is used from the session, so let's narrow it down with a selector such that we only listen to and pass down the accountID.
However, when doing this, it looks like it requires changes in PrivateNotesEditPage and PrivateNotesListPage, so we'd need to migrate those to useOnyx as well. When I tried to do that locally, I ran into type errors, I think coming from withReportOrNotFound. So I think maybe we'll want to HOLD this one on the withReportOrNotFound migration
There was a problem hiding this comment.
@roryabraham This will create a holding deadlock cycle.
There was a problem hiding this comment.
@shubham1206agra why we need to hold on this PR in withReportOrNotFound migration PR
There was a problem hiding this comment.
Ok, let me explain in some more detail what led me to say that...
Basically I started with this diff locally:
diff --git a/src/libs/ReportUtils.ts b/src/libs/ReportUtils.ts
index 78ebdd92751..511123053aa 100644
--- a/src/libs/ReportUtils.ts
+++ b/src/libs/ReportUtils.ts
@@ -4045,11 +4045,11 @@ function navigateBackAfterDeleteTransaction(backRoute: Route | undefined, isFrom
/**
* Go back to the previous page from the edit private page of a given report
*/
-function goBackFromPrivateNotes(report: OnyxEntry<Report>, session: OnyxEntry<Session>, backTo?: string) {
- if (isEmpty(report) || isEmpty(session) || !session.accountID) {
+function goBackFromPrivateNotes(report: OnyxEntry<Report>, accountID: number, backTo?: string) {
+ if (isEmpty(report) || !accountID) {
return;
}
- const currentUserPrivateNote = report.privateNotes?.[session.accountID]?.note ?? '';
+ const currentUserPrivateNote = report.privateNotes?.[accountID]?.note ?? '';
if (isEmpty(currentUserPrivateNote)) {
const participantAccountIDs = getParticipantsAccountIDsForDisplay(report);
diff --git a/src/pages/PrivateNotes/PrivateNotesEditPage.tsx b/src/pages/PrivateNotes/PrivateNotesEditPage.tsx
index ac8eb7f862b..3be994d1cbc 100644
--- a/src/pages/PrivateNotes/PrivateNotesEditPage.tsx
+++ b/src/pages/PrivateNotes/PrivateNotesEditPage.tsx
@@ -46,7 +46,7 @@ type PrivateNotesEditPageProps = WithReportAndPrivateNotesOrNotFoundProps &
report: Report;
};
-function PrivateNotesEditPage({route, personalDetailsList, report, session}: PrivateNotesEditPageProps) {
+function PrivateNotesEditPage({route, personalDetailsList, report, accountID}: PrivateNotesEditPageProps) {
const backTo = route.params.backTo;
const styles = useThemeStyles();
const {translate} = useLocalize();
@@ -117,7 +117,7 @@ function PrivateNotesEditPage({route, personalDetailsList, report, session}: Pri
>
<HeaderWithBackButton
title={translate('privateNotes.title')}
- onBackButtonPress={() => ReportUtils.goBackFromPrivateNotes(report, session, backTo)}
+ onBackButtonPress={() => ReportUtils.goBackFromPrivateNotes(report, accountID, backTo)}
shouldShowBackButton
onCloseButtonPress={() => Navigation.dismissModal()}
/>
diff --git a/src/pages/PrivateNotes/PrivateNotesListPage.tsx b/src/pages/PrivateNotes/PrivateNotesListPage.tsx
index cc7ee9f54da..71548bb81f9 100644
--- a/src/pages/PrivateNotes/PrivateNotesListPage.tsx
+++ b/src/pages/PrivateNotes/PrivateNotesListPage.tsx
@@ -43,7 +43,7 @@ type NoteListItem = {
accountID: string;
};
-function PrivateNotesListPage({report, personalDetailsList, session}: PrivateNotesListPageProps) {
+function PrivateNotesListPage({report, personalDetailsList, accountID: currentUserAccountID}: PrivateNotesListPageProps) {
const route = useRoute<RouteProp<PrivateNotesNavigatorParamList, typeof SCREENS.PRIVATE_NOTES.LIST>>();
const backTo = route.params.backTo;
const styles = useThemeStyles();
@@ -82,14 +82,14 @@ function PrivateNotesListPage({report, personalDetailsList, session}: PrivateNot
return {
reportID: report.reportID,
accountID,
- title: Number(session?.accountID) === Number(accountID) ? translate('privateNotes.myNote') : personalDetailsList?.[accountID]?.login ?? '',
+ title: currentUserAccountID === Number(accountID) ? translate('privateNotes.myNote') : personalDetailsList?.[accountID]?.login ?? '',
action: () => Navigation.navigate(ROUTES.PRIVATE_NOTES_EDIT.getRoute(report.reportID, accountID, backTo)),
brickRoadIndicator: privateNoteBrickRoadIndicator(Number(accountID)),
note: privateNote?.note ?? '',
- disabled: Number(session?.accountID) !== Number(accountID),
+ disabled: currentUserAccountID !== Number(accountID),
};
});
- }, [report, personalDetailsList, session, translate, backTo]);
+ }, [currentUserAccountID, report, personalDetailsList, translate, backTo]);
return (
<ScreenWrapper
diff --git a/src/pages/home/report/withReportAndPrivateNotesOrNotFound.tsx b/src/pages/home/report/withReportAndPrivateNotesOrNotFound.tsx
index 410af482a36..3ca623e0bc3 100644
--- a/src/pages/home/report/withReportAndPrivateNotesOrNotFound.tsx
+++ b/src/pages/home/report/withReportAndPrivateNotesOrNotFound.tsx
@@ -12,14 +12,13 @@ import NotFoundPage from '@pages/ErrorPage/NotFoundPage';
import LoadingPage from '@pages/LoadingPage';
import type {TranslationPaths} from '@src/languages/types';
import ONYXKEYS from '@src/ONYXKEYS';
-import type {Session} from '@src/types/onyx';
import {isEmptyObject} from '@src/types/utils/EmptyObject';
import type {WithReportOrNotFoundOnyxProps, WithReportOrNotFoundProps} from './withReportOrNotFound';
import withReportOrNotFound from './withReportOrNotFound';
type WithReportAndPrivateNotesOrNotFoundOnyxProps = {
- /** Session of currently logged in user */
- session: OnyxEntry<Session>;
+ /** accountID of currently logged in user */
+ accountID: number;
};
type WithReportAndPrivateNotesOrNotFoundProps = WithReportOrNotFoundProps & WithReportAndPrivateNotesOrNotFoundOnyxProps;
@@ -32,16 +31,16 @@ export default function (pageTitle: TranslationPaths) {
function WithReportAndPrivateNotesOrNotFound(props: Omit<TProps, keyof WithReportAndPrivateNotesOrNotFoundOnyxProps>, ref: ForwardedRef<TRef>) {
const {translate} = useLocalize();
const {isOffline} = useNetwork();
- const [session] = useOnyx(ONYXKEYS.SESSION);
+ const [currentUserAccountID] = useOnyx(ONYXKEYS.SESSION, {selector: (val) => val?.accountID});
const {route, report} = props;
const [reportNameValuePairs] = useOnyx(`${ONYXKEYS.COLLECTION.REPORT_NAME_VALUE_PAIRS}${report?.reportID ?? -1}`);
- const accountID = ('accountID' in route.params && route.params.accountID) || '';
+ const accountIDFromRoute = Number(('accountID' in route.params && route.params.accountID) || '');
const isPrivateNotesFetchTriggered = report?.isLoadingPrivateNotes !== undefined;
const prevIsOffline = usePrevious(isOffline);
const isReconnecting = prevIsOffline && !isOffline;
- const isOtherUserNote = !!accountID && Number(session?.accountID) !== Number(accountID);
+ const isOtherUserNote = !!accountIDFromRoute && currentUserAccountID !== accountIDFromRoute;
const isPrivateNotesFetchFinished = isPrivateNotesFetchTriggered && !report.isLoadingPrivateNotes;
- const isPrivateNotesUndefined = accountID ? report?.privateNotes?.[Number(accountID)]?.note === undefined : isEmptyObject(report?.privateNotes);
+ const isPrivateNotesUndefined = accountIDFromRoute ? report?.privateNotes?.[accountIDFromRoute]?.note === undefined : isEmptyObject(report?.privateNotes);
useEffect(() => {
// Do not fetch private notes if isLoadingPrivateNotes is already defined, or if network is offline.
@@ -75,7 +74,7 @@ export default function (pageTitle: TranslationPaths) {
return <LoadingPage title={translate(pageTitle)} />;
}
- if (shouldShowNotFoundPage) {
+ if (shouldShowNotFoundPage || !currentUserAccountID) {
return <NotFoundPage />;
}
@@ -84,7 +83,7 @@ export default function (pageTitle: TranslationPaths) {
// eslint-disable-next-line react/jsx-props-no-spreading
{...(props as TProps)}
ref={ref}
- session={session}
+ accountID={currentUserAccountID}
/>
);
}So far so good... but then we have to migrate PrivateNotesEditPage and PrivateNotesListPage to useOnyx. So on top of the previous diff, I added these changes:
diff --git a/src/pages/PrivateNotes/PrivateNotesEditPage.tsx b/src/pages/PrivateNotes/PrivateNotesEditPage.tsx
index 3be994d1cbc..071c76f8a9d 100644
--- a/src/pages/PrivateNotes/PrivateNotesEditPage.tsx
+++ b/src/pages/PrivateNotes/PrivateNotesEditPage.tsx
@@ -4,8 +4,7 @@ import {Str} from 'expensify-common';
import lodashDebounce from 'lodash/debounce';
import React, {useCallback, useMemo, useRef, useState} from 'react';
import {Keyboard} from 'react-native';
-import type {OnyxEntry} from 'react-native-onyx';
-import {withOnyx} from 'react-native-onyx';
+import {useOnyx} from 'react-native-onyx';
import FormProvider from '@components/Form/FormProvider';
import InputWrapper from '@components/Form/InputWrapper';
import HeaderWithBackButton from '@components/HeaderWithBackButton';
@@ -31,26 +30,22 @@ import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES from '@src/ROUTES';
import type SCREENS from '@src/SCREENS';
import INPUT_IDS from '@src/types/form/PrivateNotesForm';
-import type {PersonalDetailsList, Report} from '@src/types/onyx';
+import type {Report} from '@src/types/onyx';
import type {Note} from '@src/types/onyx/Report';
-type PrivateNotesEditPageOnyxProps = {
- /** All of the personal details for everyone */
- personalDetailsList: OnyxEntry<PersonalDetailsList>;
-};
-
type PrivateNotesEditPageProps = WithReportAndPrivateNotesOrNotFoundProps &
- PrivateNotesEditPageOnyxProps &
StackScreenProps<PrivateNotesNavigatorParamList, typeof SCREENS.PRIVATE_NOTES.EDIT> & {
/** The report currently being looked at */
report: Report;
};
-function PrivateNotesEditPage({route, personalDetailsList, report, accountID}: PrivateNotesEditPageProps) {
+function PrivateNotesEditPage({route, report, accountID}: PrivateNotesEditPageProps) {
const backTo = route.params.backTo;
const styles = useThemeStyles();
const {translate} = useLocalize();
+ const [personalDetailsList] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST);
+
// We need to edit the note in markdown format, but display it in HTML format
const [privateNote, setPrivateNote] = useState(
() => ReportActions.getDraftPrivateNote(report.reportID).trim() || Parser.htmlToMarkdown(report?.privateNotes?.[Number(route.params.accountID)]?.note ?? '').trim(),
@@ -178,10 +173,4 @@ function PrivateNotesEditPage({route, personalDetailsList, report, accountID}: P
PrivateNotesEditPage.displayName = 'PrivateNotesEditPage';
-export default withReportAndPrivateNotesOrNotFound('privateNotes.title')(
- withOnyx<PrivateNotesEditPageProps, PrivateNotesEditPageOnyxProps>({
- personalDetailsList: {
- key: ONYXKEYS.PERSONAL_DETAILS_LIST,
- },
- })(PrivateNotesEditPage),
-);
+export default withReportAndPrivateNotesOrNotFound('privateNotes.title')(PrivateNotesEditPage);
diff --git a/src/pages/PrivateNotes/PrivateNotesListPage.tsx b/src/pages/PrivateNotes/PrivateNotesListPage.tsx
index 71548bb81f9..cb5e9a6f51e 100644
--- a/src/pages/PrivateNotes/PrivateNotesListPage.tsx
+++ b/src/pages/PrivateNotes/PrivateNotesListPage.tsx
@@ -1,8 +1,7 @@
import type {RouteProp} from '@react-navigation/native';
import {useRoute} from '@react-navigation/native';
import React, {useCallback, useMemo} from 'react';
-import type {OnyxEntry} from 'react-native-onyx';
-import {withOnyx} from 'react-native-onyx';
+import {useOnyx} from 'react-native-onyx';
import type {ValueOf} from 'type-fest';
import {AttachmentContext} from '@components/AttachmentContext';
import HeaderWithBackButton from '@components/HeaderWithBackButton';
@@ -20,19 +19,13 @@ import CONST from '@src/CONST';
import ONYXKEYS from '@src/ONYXKEYS';
import ROUTES from '@src/ROUTES';
import type SCREENS from '@src/SCREENS';
-import type {PersonalDetailsList, Report} from '@src/types/onyx';
+import type {Report} from '@src/types/onyx';
-type PrivateNotesListPageOnyxProps = {
- /** All of the personal details for everyone */
- personalDetailsList: OnyxEntry<PersonalDetailsList>;
+type PrivateNotesListPageProps = WithReportAndPrivateNotesOrNotFoundProps & {
+ /** The report currently being looked at */
+ report: Report;
};
-type PrivateNotesListPageProps = WithReportAndPrivateNotesOrNotFoundProps &
- PrivateNotesListPageOnyxProps & {
- /** The report currently being looked at */
- report: Report;
- };
-
type NoteListItem = {
title: string;
action: () => void;
@@ -43,13 +36,15 @@ type NoteListItem = {
accountID: string;
};
-function PrivateNotesListPage({report, personalDetailsList, accountID: currentUserAccountID}: PrivateNotesListPageProps) {
+function PrivateNotesListPage({report, accountID: currentUserAccountID}: PrivateNotesListPageProps) {
const route = useRoute<RouteProp<PrivateNotesNavigatorParamList, typeof SCREENS.PRIVATE_NOTES.LIST>>();
const backTo = route.params.backTo;
const styles = useThemeStyles();
const {translate} = useLocalize();
const getAttachmentValue = useCallback((item: NoteListItem) => ({reportID: item.reportID, accountID: Number(item.accountID), type: CONST.ATTACHMENT_TYPE.NOTE}), []);
+ const [personalDetailsList] = useOnyx(ONYXKEYS.PERSONAL_DETAILS_LIST);
+
/**
* Gets the menu item for each workspace
*/
@@ -115,10 +110,4 @@ function PrivateNotesListPage({report, personalDetailsList, accountID: currentUs
PrivateNotesListPage.displayName = 'PrivateNotesListPage';
-export default withReportAndPrivateNotesOrNotFound('privateNotes.title')(
- withOnyx<PrivateNotesListPageProps, PrivateNotesListPageOnyxProps>({
- personalDetailsList: {
- key: ONYXKEYS.PERSONAL_DETAILS_LIST,
- },
- })(PrivateNotesListPage),
-);
+export default withReportAndPrivateNotesOrNotFound('privateNotes.title')(PrivateNotesListPage);But now I get a type error on the last line of PrivateNotesEditPage, which ultimately is Property navigation is missing in type.... In other words, the type problem is that withReportOrNotFound adds the route prop but not the navigation prop. So that problem needs to be fixed in withReportOrNotFound.
There was a problem hiding this comment.
Now that the other PR is merged, @nkdengineer can we address this comment from Rory?
It looks like only accountID is used from the session, so let's narrow it down with a selector such that we only listen to and pass down the accountID.
There was a problem hiding this comment.
@akinwale @youssef-lr i updated, please check again
|
@nkdengineer This should be unblocked now |
|
I'll update today |
|
@akinwale resolved conflict, please check again |
|
@youssef-lr friendly bump |
|
@nkdengineer I left you a comment here https://github.com/Expensify/App/pull/49238/files#r1803867994 |
youssef-lr
dismissed
roryabraham’s stale review
requested changes have been implemented
|
Rory is OOO. Merging! |
|
@youssef-lr looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
|
All checks passed. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
42 tasks
|
🚀 Deployed to staging by https://github.com/youssef-lr in version: 9.0.53-0 🚀
|
|
🚀 Deployed to staging by https://github.com/youssef-lr in version: 9.0.53-0 🚀
|
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.0.53-1 🚀
|
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 9.0.53-1 🚀
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
Fixed Issues
$ #49106
PROPOSAL: #49106 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android.mov
Android: mWeb Chrome
android-mweb.mov
iOS: Native
ios.mov
iOS: mWeb Safari
ios-mweb.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov